Fix dogfood detector edge cases#184
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThree independent fixes across build-tool detectors: the Gradle detector reads ChangesGradle: root project name from settings files
Maven: ensure mvnw is executable
SBT: gate native detector on dependency-graph plugin
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Bomly Diff SummaryCompared Overview
Dependency Changes✅ No dependency changes. Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes (or Policy Findings✅ No policy differences were identified. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/detectors/sbt/sbt_native.go (1)
49-52: ⚡ Quick winUse compact formatted DEBUG message for this two-value log.
At Line 49-52, this log uses two structured fields; your Go logging guideline prefers a compact
fmt.Sprintf(...)message in that case.Suggested change
- d.logger().Debug("sbt native detector skipped: dependencyTree task is unavailable for this sbt version", - zap.String("working_dir", workingDir), - zap.String("sbt_version", sbtVersion(workingDir)), - ) + d.logger().Debug(fmt.Sprintf( + "sbt native detector skipped: dependencyTree task is unavailable for this sbt version (working_dir=%s, sbt_version=%s)", + workingDir, + sbtVersion(workingDir), + ))As per coding guidelines, "Prefer compact one-line messages with
fmt.Sprintf(...)for logs with one or two fields; prefer structured zap fields when a log carries several values or benefits from machine-readable context".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/detectors/sbt/sbt_native.go` around lines 49 - 52, The Debug log statement in the sbt native detector is using structured zap fields for only two values (working_dir and sbt_version). According to Go logging guidelines, logs with one or two fields should use compact fmt.Sprintf formatted messages instead of structured zap fields. Refactor the d.logger().Debug() call to embed the workingDir and sbtVersion(workingDir) values directly into the message string using fmt.Sprintf, removing the separate zap.String field parameters.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/detectors/gradle/detector_test.go`:
- Around line 233-237: Remove the inline fake Gradle binary creation code that
uses os.WriteFile to create the gradlePath fixture in this test. Instead,
integrate this Gradle script setup into the shared TestMain function fixture
setup in the test file (following the pattern used in
internal/cli/root_test_main_test.go for other fake binaries like npm and go),
then have the test reference the centralized fixture rather than creating it
locally. This ensures all fake binary setup for Gradle is consistent and
centralized in TestMain.
In `@internal/detectors/sbt/sbt_native.go`:
- Around line 162-173: The hasDependencyGraphPlugin function uses
strings.Contains to check for "sbt-dependency-graph" in the file content, which
matches both actual plugin declarations and commented-out text, causing false
positives. Instead of simple string matching, parse the file content more
carefully to identify only actual plugin declarations by checking for the proper
SBT syntax patterns (such as addSbtPlugin directives) rather than matching any
occurrence of the plugin name string. This will prevent commented-out or
incidental mentions from incorrectly triggering native mode enablement.
---
Nitpick comments:
In `@internal/detectors/sbt/sbt_native.go`:
- Around line 49-52: The Debug log statement in the sbt native detector is using
structured zap fields for only two values (working_dir and sbt_version).
According to Go logging guidelines, logs with one or two fields should use
compact fmt.Sprintf formatted messages instead of structured zap fields.
Refactor the d.logger().Debug() call to embed the workingDir and
sbtVersion(workingDir) values directly into the message string using
fmt.Sprintf, removing the separate zap.String field parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3db4a184-d4da-44ec-a229-37f719e9d3ba
📒 Files selected for processing (6)
internal/detectors/gradle/detector.gointernal/detectors/gradle/detector_test.gointernal/detectors/maven/detector.gointernal/detectors/maven/detector_test.gointernal/detectors/sbt/detector_test.gointernal/detectors/sbt/sbt_native.go
Comment on lines
+233
to
+237
| gradlePath := filepath.Join(projectDir, "gradle-fixture") | ||
| script := "#!/bin/sh\ncat <<'OUT'\nruntimeClasspath - Runtime classpath of source set 'main'.\n\\--- org.springframework:spring-core:6.1.1\nOUT\n" | ||
| if err := os.WriteFile(gradlePath, []byte(script), 0o755); err != nil { | ||
| t.Fatalf("write gradle fixture: %v", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Move fake Gradle binary setup into shared TestMain fixtures.
Line 233-Line 237 create a fake Gradle executable inline; this should be centralized in the shared TestMain fake-binary setup to keep fixture behavior consistent across detector tests.
As per coding guidelines, "Fake binaries (npm, go, Gradle, plugin) must be built in TestMain — see internal/cli/root_test_main_test.go."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/detectors/gradle/detector_test.go` around lines 233 - 237, Remove
the inline fake Gradle binary creation code that uses os.WriteFile to create the
gradlePath fixture in this test. Instead, integrate this Gradle script setup
into the shared TestMain function fixture setup in the test file (following the
pattern used in internal/cli/root_test_main_test.go for other fake binaries like
npm and go), then have the test reference the centralized fixture rather than
creating it locally. This ensures all fake binary setup for Gradle is consistent
and centralized in TestMain.
Source: Coding guidelines
Comment on lines
+162
to
+173
| func hasDependencyGraphPlugin(workingDir string) bool { | ||
| for _, name := range []string{ | ||
| filepath.Join("project", "plugins.sbt"), | ||
| filepath.Join("project", "build.sbt"), | ||
| "build.sbt", | ||
| } { | ||
| raw, err := os.ReadFile(filepath.Join(workingDir, name)) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if strings.Contains(string(raw), "sbt-dependency-graph") { | ||
| return true |
Contributor
There was a problem hiding this comment.
Plugin detection is too broad and can false-positive on comments/text.
At Line 172, strings.Contains(..., "sbt-dependency-graph") treats any mention (including comments) as plugin presence, which can incorrectly keep native mode enabled for old SBT projects.
Suggested change
+var sbtDependencyGraphPluginDecl = regexp.MustCompile(`(?m)^\s*addSbtPlugin\([^)\n]*"sbt-dependency-graph"`)
+
func hasDependencyGraphPlugin(workingDir string) bool {
for _, name := range []string{
filepath.Join("project", "plugins.sbt"),
filepath.Join("project", "build.sbt"),
"build.sbt",
} {
raw, err := os.ReadFile(filepath.Join(workingDir, name))
if err != nil {
continue
}
- if strings.Contains(string(raw), "sbt-dependency-graph") {
+ if sbtDependencyGraphPluginDecl.Match(raw) {
return true
}
}
return false
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/detectors/sbt/sbt_native.go` around lines 162 - 173, The
hasDependencyGraphPlugin function uses strings.Contains to check for
"sbt-dependency-graph" in the file content, which matches both actual plugin
declarations and commented-out text, causing false positives. Instead of simple
string matching, parse the file content more carefully to identify only actual
plugin declarations by checking for the proper SBT syntax patterns (such as
addSbtPlugin directives) rather than matching any occurrence of the plugin name
string. This will prevent commented-out or incidental mentions from incorrectly
triggering native mode enablement.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mvnwwrappers before execution, matching Gradle wrapper handlingrootProject.namefrom Gradle settings files for the root package instead of Bomly temporary checkout directory namesdependencyTreeis unavailable unlesssbt-dependency-graphis configuredValidation
Summary by CodeRabbit
Bug Fixes
Improvements
settings.gradleorsettings.gradle.kts, improving dependency graph representation.Tests